-
Notifications
You must be signed in to change notification settings - Fork 126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
split docs/produtct dependencies #621
Conversation
/genai-describe |
|
||
```ts | ||
const data = await workspace.readXML("data.xml") | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The readXML
function is added without a description of its behavior or use cases.
generated by pr-docs-review-commit
missing_example_description
"@_attr": "1", | ||
"child": {} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code fence around the JSON example should be js or
javascript, not ```json, since the content is not valid JSON due to the unquoted keys.
generated by pr-docs-review-commit
incorrect_code_fence
const file = await fs.readText(f) | ||
const res = XMLParse(file.content) | ||
return res | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no error handling for the XML parsing. If the XML is malformed, this could cause an unhandled exception.
generated by pr-review-commit
missing_error_handling
@@ -90,6 +90,7 @@ export async function createPromptContext( | |||
const workspace: WorkspaceFileSystem = { | |||
readText: (f) => runtimeHost.workspace.readText(f), | |||
readJSON: (f) => runtimeHost.workspace.readJSON(f), | |||
readXML: (f) => runtimeHost.workspace.readXML(f), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no error handling for the XML reading. If the XML is malformed or the file does not exist, this could cause an unhandled exception.
generated by pr-review-commit
missing_error_handling
ignoreDeclaration: true, | ||
parseAttributeValue: true, | ||
...(options || {}), | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no error handling for the XML parsing. If the XML is malformed, this could cause an unhandled exception.
generated by pr-review-commit
missing_error_handling
ignoreDeclaration: true, | ||
parseAttributeValue: true, | ||
...(options || {}), | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The attribute name prefix is hardcoded to "@_". This could lead to confusion or unexpected behavior if other parts of the codebase expect a different prefix. Consider making this a configurable option. 🔄
generated by pr-review-commit
hardcoded_values
test("parse attribute", () => { | ||
const x = XMLParse('<root a="1"><b>2</b></root>') | ||
assert.deepStrictEqual(x, { root: { b: 2, "@_a": 1 } }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test case for XML parsing only checks for a single attribute. It would be more robust to include multiple attributes in the test case to ensure the parser can handle them correctly. 🧪
generated by pr-review-commit
incomplete_test
|
||
```ts | ||
const data = await workspace.readXML("data.xml") | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The readXML
function is added but there is no explanation or details provided about its usage or behavior.
generated by pr-docs-review-commit
missing_documentation
"@_attr": "1", | ||
"child": {} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example provided for parsers.XML
function is in JSON format, which is inconsistent with the context that should show an XML parsing example.
generated by pr-docs-review-commit
incorrect_example
@@ -87,7 +87,7 @@ Options: | |||
-td, --test-delay <string> delay between tests in seconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The option name --promptfoo-version
seems incorrect. It should likely be --prompt-version
.
generated by pr-docs-review-commit
option_name
|
||
```ts | ||
const data = await workspace.readXML("data.xml") | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new readXML
function has been documented. Ensure that the implementation matches the documentation.
generated by pr-docs-review-commit
new_feature
"@_attr": "1", | ||
"child": {} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parsers.XML
function documentation has been updated with more details on how attributes are parsed. Ensure that the implementation matches the documentation.
generated by pr-docs-review-commit
xml_parsing_details
@@ -249,7 +249,7 @@ export async function cli() { | |||
.alias("parsers") | |||
.description("Parse various outputs") | |||
parser | |||
.command("fence <language>") | |||
.command("fence <language> <file>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parseFence
function now requires two arguments, but only one is being passed here. Please pass the required file
argument. 📝
generated by pr-review-commit
missing_argument
const fences = extractFenced(stdin || "").filter( | ||
export async function parseFence(language: string, file: string) { | ||
const res = await parsePdf(file) | ||
const fences = extractFenced(res.content || "").filter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function parsePdf
is not defined in this file or imported from another module. Please ensure that this function is defined before it is called. 🧐
generated by pr-review-commit
undefined_function
@@ -90,6 +90,7 @@ export async function createPromptContext( | |||
const workspace: WorkspaceFileSystem = { | |||
readText: (f) => runtimeHost.workspace.readText(f), | |||
readJSON: (f) => runtimeHost.workspace.readJSON(f), | |||
readXML: (f) => runtimeHost.workspace.readXML(f), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The readXML
function is not defined in the runtimeHost.workspace
object. Please ensure that this function is defined before it is called. 🕵️♀️
generated by pr-review-commit
missing_function
@@ -292,10 +292,10 @@ Options: | |||
-h, --help display help for command | |||
|
|||
Commands: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command usage for fence
is missing the <file>
argument.
generated by pr-docs-review-commit
command_usage
pdf <file> Parse a PDF into text | ||
docx <file> Parse a DOCX into texts | ||
html-to-text [file] Parse an HTML file into text | ||
html-to-text <file> Parse an HTML file into text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command usage for html-to-text
is missing the <file>
argument.
generated by pr-docs-review-commit
command_usage
@@ -305,7 +305,7 @@ Commands: | |||
### `parse fence` | |||
|
|||
``` | |||
Usage: genaiscript parse fence [options] <language> | |||
Usage: genaiscript parse fence [options] <language> <file> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command usage for parse fence
is missing the <file>
argument.
generated by pr-docs-review-commit
command_usage
@@ -338,7 +338,7 @@ Options: | |||
### `parse html-to-text` | |||
|
|||
``` | |||
Usage: genaiscript parse html-to-text [options] [file] | |||
Usage: genaiscript parse html-to-text [options] <file> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command usage for parse html-to-text
is missing the <file>
argument.
generated by pr-docs-review-commit
command_usage
"@_attr": "1", | ||
"child": {} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parsers.XML
function documentation has been updated with new information about attribute naming conventions. Ensure that the implementation matches the documentation.
generated by pr-docs-review-commit
new_feature
const fences = extractFenced(stdin || "").filter( | ||
export async function parseFence(language: string, file: string) { | ||
const res = await parsePdf(file) | ||
const fences = extractFenced(res.content || "").filter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function parsePdf
is called here, but it is not defined or imported in this file. Please check the function name or import it.
generated by pr-review-commit
wrong_function_call
allowBooleanAttributes: true, | ||
ignoreDeclaration: true, | ||
parseAttributeValue: true, | ||
...(options || {}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The options object is being overwritten here, which could lead to unexpected behavior if options are passed to the XMLParse
function. Consider merging the default options with the passed options instead of overwriting them.
generated by pr-review-commit
unnecessary_code
allowBooleanAttributes: true, | ||
ignoreDeclaration: true, | ||
parseAttributeValue: true, | ||
...(options || {}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The XML parser is now configured to parse attributes, which might lead to unexpected behavior if the rest of the codebase is not prepared to handle attributes.
generated by pr-review-commit
unexpected_behavior
@@ -264,7 +264,7 @@ export async function cli() { | |||
.action(parseDOCX) | |||
|
|||
parser | |||
.command("html-to-text [file]") | |||
.command("html-to-text <file>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function parseHTMLToText
is now expecting one argument, but none is being provided. Please provide the required argument.
generated by pr-review-commit
missing_argument
/genai-describe |
allowBooleanAttributes: true, | ||
ignoreDeclaration: true, | ||
parseAttributeValue: true, | ||
...(options || {}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The XML parser options have been changed. This could potentially break existing functionality if the parser is used elsewhere in the codebase.
generated by pr-review-commit
unexpected_change
Split workspace to avoid docs/slides pollute genaiscript dependency list
install:force
has been added to thepackage.json
for forcefully reinstalling dependencies.astro
version in dependencies has been upgraded from^4.13.1
to^4.14.2
.@astrojs/check
,@astrojs/starlight
, andtypescript
.commands.md
, the defaultpromptfoo
version has been updated from^0.75.2
to0.78.0
.files.md
andparsers.md
to include information aboutreadXML
.yarn.lock
- has been introduced into the project.devDependencies
.fence
,html-to-text
, and their parse commands incommands.md
.files.md
andparsers.md
, adding new methods and examples associated with parsing XML content.